fix(cli): detect video motion in sweep guard#2308
Conversation
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at e895092a.
Right generalization at the right layer. The element-agnostic size resolution videoWidth || width || rect.width correctly handles both cases: <video> gets its intrinsic dimensions (post-metadata), <canvas> gets its internal .width/.height since videoWidth is undefined on it. Falls back to rect.width for the degenerate case, which composes with the earlier fixture test for canvases without valid .width.
The tainted/CORS story is preserved and load-bearing: getImageData throws on cross-origin video without CORS → outer try/catch returns "x" → hashes to constant. Comment explicitly guarantees no new false negatives ("never a new false negative for DOM-motion compositions"). Cross-origin video is silent-motion, but that's a symmetric limitation to cross-origin canvas which the pre-PR code already had.
Verified there's no sibling sweep_static detection elsewhere in the codebase — grep shows one call site at layout-audit.browser.js:938, one detector at check.test.ts:1064-1110. No other place where "canvas but not video" gets exercised, so nothing left narrower after this change.
Test coverage nails the exact bug: mocked getImageData returns a monotone Uint8ClampedArray filled with pixelValue, pixelValue flips 20→220 between collect() calls, assertion is after !== before. Ratchets the "video pixel motion advances the fingerprint" invariant.
Nits
-
🟡 Prefix rename
c:→p:in the fingerprint string is a fingerprint-shape break. Not a bug because these fingerprints are session-scoped (recomputed each audit run, not cached across releases), but if any downstream consumer does pin to thec:prefix (e.g. hypothetical golden-fingerprint fixtures on disk), they'd need updating. A quick grep for"c:"and fingerprint string patterns confirms no such consumers, so this is a nit. -
🟡
<iframe>with video inside is out of scope (cross-frame document boundary), and probably worth documenting in the comment so a future reader doesn't extend the selector to"canvas, video, iframe"and get bitten by SOP.
LGTM.
Addressed in
Verified with fixed-string searches for |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 reviewed at 41e5a13f, delta from R1 head e895092a.
R1 nit on iframe/SOP scope addressed — layout-audit.browser.js:903-904 now documents the intentional exclusion inline (media inside iframes lives in a separate document; cross-origin frames are SOP-inaccessible). No behavioral change; the comment sits next to mediaPixelHash where a future reader would look.
LGTM from my side.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving on Rames-D's go (R2 delta clean, nit round addressed) + Magi's CI-green confirmation. Verified at final head 41e5a13f: CI green, no changes-requested, mergeable.
3 participants
What
<video>pixels in the layout sweep fingerprintWhy
hyperframes checkreportssweep_staticfor seek-driven hard-cut/video compositions when DOM geometry and opacity stay fixed. Snapshots and renders seek correctly, but the layout gate only observed DOM geometry (plus canvas pixels), so video-only motion looked frozen.Reproduction
Confirmed on 0.7.55 with a 4s direct-root
<video>, registered paused GSAP timeline, static title, and advancing video frames.checkreturnedsweep_static; snapshots/render advanced correctly.Verification
bun run --cwd packages/cli test src/commands/layout-audit.browser.test.ts(31 passed)bun run --cwd packages/cli typecheckcheckagainst the 4s video repro:ok=true, nosweep_staticgit diff --check